Skip to content

fix: return logs for validator tests #109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 31, 2025
Merged

fix: return logs for validator tests #109

merged 3 commits into from
Oct 31, 2025

Conversation

@taco-paco
Copy link
Contributor

@taco-paco taco-paco commented Oct 17, 2025 •
edited by coderabbitai bot
Loading

⚠️ NOTE: Use notes like this to emphasize something important about the PR.

This could include other PRs this PR is built on top of; API breaking changes; reasons for why the PR is on hold; or anything else you would like to draw attention to.

Status Type ⚠️ Core Change Issue
Ready - No Link

Problem

What problem are you trying to solve?
Some of the tests in validator rely on logs by dlp. Those logs not longer present in fast branch

Solution

How did you solve the problem?
Adding logs back under unit_test_config feature

Before & After Screenshots

Insert screenshots of example code output

BEFORE:
[insert screenshot here]

AFTER:
[insert screenshot here]

Other changes (e.g. bug fixes, small refactors)

Deploy Notes

Notes regarding deployment of the contained body of work. These should note any
new dependencies, new scripts, etc.

New scripts:

  • script : script details

New dependencies:

  • dependency : dependency details

Summary by CodeRabbit

  • Chores
    • Added an opt-in "logging" feature flag that enables conditional runtime logs for instruction processing.
    • Removed a previously unconditional runtime log so behavior is unchanged unless the logging feature is enabled.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 17, 2025 •
edited
Loading

Walkthrough

Adds a crate feature logging; moves a runtime log into a feature-gated block in fast_process_instruction (enabled by logging) and removes an unconditional log from slow_process_instruction. No control-flow or API changes.

Changes

Cohort / File(s) Summary
Feature manifest
Cargo.toml		 Adds a new crate feature logging in the [features] section.
Conditional logging changes
src/lib.rs		 Adds #[cfg(feature = "logging")] import and log in fast_process_instruction to log processed discriminator variants; removes the unconditional log from slow_process_instruction. No control-flow changes.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller
    participant Processor
    participant FastProc as fast_process_instruction
    participant Logger

    Caller->>Processor: invoke processing
    Processor->>FastProc: call fast_process_instruction(discriminator, ...)
    alt logging feature enabled
        FastProc->>Logger: log("processed discriminator", discriminator)
    else logging feature disabled
        FastProc-->>Processor: proceed without logging
    end
    FastProc-->>Processor: return result
    Processor-->>Caller: return
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify logging feature name and that it's listed correctly in Cargo.toml.
  • Confirm #[cfg(feature = "logging")] gates compile-only logging code and that imports (e.g., msg/logger) are feature-gated consistently.
  • Check that removing the unconditional log from slow_process_instruction doesn't remove required diagnostics or tests that rely on that output.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "fix: return logs for validator tests" accurately captures the main objective of the changeset. The modifications introduce a "logging" feature flag to conditionally enable logging statements that were previously absent, which directly aligns with restoring logs for validator tests as described in the PR objectives. The title is concise, clear, and specific enough for a teammate scanning history to understand the primary change without ambiguity.
Description Check ✅ Passed The PR description follows the provided template structure and includes the essential sections: a status table, a Problem section explaining that validator tests rely on logs no longer present on the fast branch, and a Solution section indicating logs are being added under a feature flag. While the Type field in the status table shows "-" rather than a specific category, and optional sections like deploy notes contain only placeholders, the core information needed to understand the PR's purpose and implementation is present and clearly articulated. One note: there is a discrepancy between the description mentioning "unit_test_config" feature and the actual code changes showing a "logging" feature flag, though this appears to be a naming detail rather than a structural or completeness issue with the description itself.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/return-logs-for-tests-pr
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 52001d4 and c220dc3.

📒 Files selected for processing (1)
  • src/lib.rs (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T11:45:25.802Z 
Learnt from: snawaz
PR: magicblock-labs/delegation-program#107
File: src/entrypoint.rs:141-144
Timestamp: 2025-10-15T11:45:25.802Z
Learning: In the delegation-program codebase, prefer using `log!` (from pinocchio_log) over `msg!` for error and panic scenarios in the entrypoint code, as per maintainer preference.

Applied to files:

  • src/lib.rs
🧬 Code graph analysis (1)
src/lib.rs (1)
src/entrypoint.rs (1)
  • entrypoint (13-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: install
🔇 Additional comments (2)
src/lib.rs (2)

11-12: LGTM! Clean feature-gated import.

The conditional import of msg under the logging feature is correctly implemented and aligns with its usage in the code.

67-68: Feature-gated logging correctly implemented and verified.

The discriminator logging is properly placed in fast_process_instruction and guarded by the logging feature flag. Verification confirms that validator tests do not assert on discriminator logs for slow-path instructions (InitValidatorFeesVault, InitProtocolFeesVault, WhitelistValidatorForProgram, etc.), making this implementation suitable for test requirements while avoiding CU overhead in production builds.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

snawaz
snawaz requested changes Oct 18, 2025
View reviewed changes
GabrielePicco
GabrielePicco requested changes Oct 22, 2025
View reviewed changes
@taco-paco taco-paco force-pushed the fix/return-logs-for-tests-pr branch from 75fde9a to fa8e616 Compare October 30, 2025 07:22
@GabrielePicco GabrielePicco mentioned this pull request Oct 30, 2025
Merged
@GabrielePicco GabrielePicco requested a review from snawaz October 30, 2025 11:47
snawaz
snawaz approved these changes Oct 30, 2025
View reviewed changes
Copy link
Contributor

@snawaz snawaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Let's merge. 🎉

And thanks for suggesting an alternative testing approach in #115. Overall, it feels much better. 👍

@taco-paco taco-paco merged commit 81daff2 into main Oct 31, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@snawaz snawaz snawaz approved these changes

@GabrielePicco GabrielePicco GabrielePicco approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@taco-paco @snawaz @GabrielePicco